Skip to content

Conversation

@NaccOll
Copy link

@NaccOll NaccOll commented Aug 10, 2025

relate: #6897
fix: #6720 #6700

In multi-folder workspaces, mimic the history and index status, binding the dialog to the CWD when creating a new ChatView. This will prevent CWD switching during a conversation and further confusion.


Important

Introduces getCurrentCwd() in webviewMessageHandler.ts to handle CWD in multi-folder workspaces, ensuring consistent directory usage for command operations.

  • Behavior:
    • Introduces getCurrentCwd() in webviewMessageHandler.ts to determine the current working directory in multi-folder workspaces.
    • Uses getCurrentCwd() in place of vscode.workspace.workspaceFolders[0].uri.fsPath to set workspaceFolder and workspaceRoot.
    • Updates requestCommands, openCommandFile, deleteCommand, and createCommand cases to use getCurrentCwd() for command operations.
  • Misc:
    • Fixes potential issues with CWD switching during conversations in multi-folder workspaces.

This description was created by Ellipsis for a9f374d. You can customize this summary. It will automatically update as commits are pushed.

@NaccOll NaccOll requested review from cte, jr and mrubens as code owners August 10, 2025 16:35
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Aug 10, 2025
@NaccOll
Copy link
Author

NaccOll commented Aug 10, 2025

@mrubens

There are still many areas of webviewMessageHandler that need to be modified.

I suggest you review this pull request first. Once it's approved and merged, I'll continue working on it.

Copy link

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! I've reviewed the changes and found several issues that need attention before this can be merged. The implementation appears incomplete and could lead to inconsistent behavior in multi-folder workspaces.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 10, 2025
@NaccOll NaccOll force-pushed the hotfix-multiple-folder-workspace-slash branch from a9f374d to c7d83ca Compare August 10, 2025 16:47
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Aug 12, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Aug 12, 2025
@daniel-lxs
Copy link
Member

Thank you @NaccOll! I was testing this adding and removing slash commands with 2 workspaces and it seems to work fine until I try deleting and creating the commands multiple times. It seems that different commands appear and disappear depending on which workspace Roo Code is currently selecting. Maybe the logic that populates the commands omits the other workspaces occasionally? Can you take a look?

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Changes Requested] in Roo Code Roadmap Aug 14, 2025
@NaccOll
Copy link
Author

NaccOll commented Aug 15, 2025

Thank you @NaccOll! I was testing this adding and removing slash commands with 2 workspaces and it seems to work fine until I try deleting and creating the commands multiple times. It seems that different commands appear and disappear depending on which workspace Roo Code is currently selecting. Maybe the logic that populates the commands omits the other workspaces occasionally? Can you take a look?

You're right. The CWD switch should only be triggered when the user clicks on a new ChatView, to avoid inconsistencies with the code index and task history.

Currently, the Slash Command Panel is determined by the folder where the file is located when the user clicks it. While this seems convenient, the inconsistency can cause user confusion. Furthermore, when the Slash Command Panel pops up and you switch files between folders, adding a Slash Command while doing so will add it to the new folder, and the panel contents will be refreshed to the new directory, which can also cause confusion.

@NaccOll NaccOll force-pushed the hotfix-multiple-folder-workspace-slash branch from c7d83ca to 8efee19 Compare August 15, 2025 08:24
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Aug 15, 2025
@NaccOll NaccOll force-pushed the hotfix-multiple-folder-workspace-slash branch from c3a22cb to c40ee4e Compare August 15, 2025 10:04
@NaccOll
Copy link
Author

NaccOll commented Aug 15, 2025

@daniel-lxs

I have made significant improvements to the CWD binding for webview and provider. The WorkspacePath now only switches when users click the plus button.

Except for saveImage and custom modes, all confusion should now be avoidable. The first phase of work for #6897 is nearly complete.

@daniel-lxs daniel-lxs moved this from PR [Changes Requested] to PR [Needs Prelim Review] in Roo Code Roadmap Aug 15, 2025
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on multi-folder workspace support! I've been looking through the changes and have some questions about the approach.

The main changes seem to be adding a getCurrentCwd() helper and storing workspace paths per task, which makes sense. But I'm curious about a few implementation details that could affect reliability.

Left some questions inline to better understand the intent and potential edge cases.

TelemetryService.instance.captureTitleButtonClicked("plus")

await visibleProvider.removeClineFromStack()
await visibleProvider.refreshWorkspace()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we're calling refreshWorkspace() here after removing from the stack. What's the purpose of refreshing the workspace at this point? Is it to pick up the active editor's workspace folder?

Also wondering - if getWorkspacePath() returns an empty string during this refresh (no editor open), would that cause issues for the new task that's about to be created?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the cmd property of the provider are the core modifications. The purpose is to switch the workspacePath only when the user clicks the plus sign, and to make all file-related operations rely on the Task's cwd first, and then the provider's cwd. In this way, when working in a multi-folder workspace, whether it is indexing, slash command or other actions, the user must switch the workspacePath by clicking the plus sign.

As for getting an empty string, this is not a problem.
getWorkspacePath basically cannot return an empty string. It first gets the workspacePath where the currently opened file is located, then the first folder of the multi-root workspace, and finally the empty string as the default value. When it is an empty string, it is only when the user does not open any directory. At this time, RooCode can be used to initiate tasks and conduct simple questions and answers, but it actually has no effect. All activities related to workspacePath are prohibited.

In fact, the source of all the confusion lies in the abuse of getWorkspacePath. The current main uses getWorkspacePath and vscode.workspace.workspaceFolders[0] extensively, which results in different functions sometimes forcing the first directory to be obtained, and sometimes based on the current active file directory. However, the cwd of the webview may be the one left before it switched the active file, which often leads to the inconsistency between the ui in the root workspace and the cwd of different background jobs, causing confusion.

await this.postStateToWebview()
}

async refreshWorkspace() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refreshWorkspace method updates currentWorkspacePath but doesn't seem to notify anything about the change. Should this also post a message to the webview or update the state somehow?

Also, what happens if getWorkspacePath() throws here? There's no error handling.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, no processing is needed here, the behavior here is the same as before. It's just that before, the workspacePath (the directory where the current active file is located) was dynamically obtained through the provider's cwd method, but now it refreshes the provider's currentWorkspacePath first and then pushes it to the webview.postStateToWebview is called after refreshWorkspace. Maybe I can combine them into one method

this.log(`Failed to initialize cloud profile sync: ${error}`)
})

this.currentWorkspacePath = getWorkspacePath()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During early initialization, getWorkspacePath() might return an empty string if no workspace is loaded yet. Is storing an empty string intentional here?

I'm wondering if this could cause issues downstream - for example, when creating a task with an empty workspace path.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with other

this.workspacePath = parentTask
? parentTask.workspacePath
: getWorkspacePath(path.join(os.homedir(), "Desktop"))
: (workspacePath ?? getWorkspacePath(path.join(os.homedir(), "Desktop")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that if workspacePath is an empty string (which can happen from the provider), it gets stored as-is because empty string isn't nullish. This bypasses the Desktop fallback.

Was this intentional? It seems like we'd want to use the Desktop fallback when there's no valid workspace path.

Maybe consider: workspacePath && workspacePath.trim() ? workspacePath : getWorkspacePath(...)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behavior is the same as the main branch.

Previously, when a user created a task without opening any folders, path.join(os.homedir(), "Desktop") would be used as the workspace. This was completely meaningless because other functions often bypassed the Task's cwd and used getWorkspacePath to obtain the cwd. Now, I prioritize the Task's cwd, followed by the Provider's cwd, and finally using getWorkspacePath, which actually changes the original behavior.

Previously, when using getWorkspacePath to obtain the workspace path, an empty string would be returned because the user had no folders open, even if the Task's cwd was path.join(os.homedir(), "Desktop") . However, after my changes, if I didn't do this, many functions would assume that the workspacePath existed, allowing users to perform slash commands and MCP project settings without opening a folder, which was undoubtedly confusing. At the same time, the code index would not be initialized because it couldn't detect any folders.


get cwd() {
return getWorkspacePath()
return this.currentWorkspacePath || getWorkspacePath()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cwd getter now returns currentWorkspacePath || getWorkspacePath(). This means it could return different values over time as the active editor changes.

Is this intentional? I'm wondering if this could cause confusion when the cwd changes unexpectedly during a task.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have misunderstood here. It is because cwd directly called getWorkspacePath() before that caused the confusion of multiple folders. Now that it is managed uniformly through currentWorkspacePath, the confusion has disappeared. As for retaining getWorkspacePath(), it is just my defensive programming to prevent errors.

}

export async function openMention(mention?: string): Promise<void> {
export async function openMention(cwd: string, mention?: string): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change to pass cwd as a parameter instead of calling getWorkspacePath() directly. This makes the function more testable and explicit about its dependencies.

Just wondering - what happens if cwd is an empty string here? Should we validate it's not empty before proceeding?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is no different than any other; it all comes down to the source of cwd . If cwd is an empty string, then getWorkspacePath , the root cause, must also return an empty string. Ultimately, I simply fixed getWorkspacePath at the moment the user clicks the plus button and tried to use it across all functions.


get cwd() {
return getWorkspacePath()
return this.providerRef?.deref()?.cwd ?? getWorkspacePath()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting change to use the provider's cwd when available. This creates a dependency cycle though - WorkspaceTracker uses provider.cwd, and provider might use WorkspaceTracker.

Is this intentional? Could this cause any issues with initialization order?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My answer is no. But this question is certainly interesting, and it will become even more useful later.

Currently, I've discovered that its only purpose is to push workspaceUpdated every 300ms and return the file list corresponding to the cwd.

workspaceUpdated is used in two places: first, as the file list carried in the context sent with the user message, and second, as an index (I used it in previous multi-workspace support, but it's no longer used, and I'll refactor the code later).

The purpose of this change is to ensure that the cwd is consistent with the provider. Otherwise, if the user's active folder changes, the file list will be refreshed to the other folder. This will cause the file list carried in the context sent by the user to be inconsistent with the cwd used in the message, so this change is necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Needs Review size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Roo does not identify .roo folder in multiroot workspace

4 participants